fix(getOptions): deprecation warn in loaderUtils #147
Conversation
joshwiens
force-pushed
the
d3viant0ne-TravisUpdates
branch
from
58510ed to
5e07d05
Compare
joshwiens
changed the title
jhnns
left a comment
jhnns
left a comment
There was a problem hiding this comment.
Is yarn.lock necessary for yarn users?
| - "rm -rf node_modules && npm install npm@3" | ||
| - "node_modules/.bin/npm install" | ||
| - "node_modules/.bin/npm test" | ||
| - yarn run travis:$JOB_PART |
There was a problem hiding this comment.
Why yarn? Our primary goal should still be npm.
There was a problem hiding this comment.
Travis uses yarn across the entire org though we do not force the use of yarn for users.
Hence the extra command below. With the coming defaults upgrade, more parts will be added to the matrix.
There was a problem hiding this comment.
For users that don't want to use yarn, the lock file is completely benign ( it's treated as a binary file in git )
There was a problem hiding this comment.
Ok ... and could you describe what this change in the travis.yml was about? It's not related to the loader-utils change and I haven't worked with travis matrices so far, so I'm a bit puzzled :)
I don't want to bother you, I just want to understand the change 😁
There was a problem hiding this comment.
The loader-utils update forces node 4.3 as a minimum supported version ( old travis file tests against 0.10.x & 0.12.x). Given I had to update it anyway, I grabbed the travis file that will be added as part of the pending webpack-defaults upgrade ( it's used in other libs already ).
So while it's not directly related, it is a side effect of the forced drop of the older node versions.
There was a problem hiding this comment.
If you look at the two commented out portions of the matrix "lint & coverage" this setup also allows us to test against older / newer versions of webpack by adding another include with a specific WEBPACK_VERSION on whatever node version we want. These can also be set to Can Fail in the matrix for testing against a beta build / nightly without failing the pull request outright.
WEBPACK_VERSION="2.2.0" is removed from the three test parts becuase the test suite fails schema validation. Hence the comment on the bottom of the file.
| "scripts": { | ||
| "test": "node --no-deprecation node_modules/.bin/_mocha -R spec", | ||
| "test": "npm run travis:test", | ||
| "travis:test": "node --no-deprecation node_modules/.bin/_mocha -R spec", |
There was a problem hiding this comment.
- Why is there an extra command for travis:test? We should just define
test - I know that this has been there before, but why
--no-deprecation?
There was a problem hiding this comment.
Why is there an extra command for travis:test? We should just define test
Look at the .travis.yml. The matrix executes different parts across different supported node versions. A part is defined as travis:<part>
I know that this has been there before, but why --no-deprecation
I have no idea, left it alone mainly becuase I don't know why it was there in the first place.
I can either remove it now or it will be removed on the defaults upgrade to Jest. Your choice.
|
I think, this PR is not complete because it doesn't make a deep copy of the options. For instance, the webpack resolver plugin will be added for every loader invocation to the same config. This is a great example why we deprecated However, I'll merge it and make some changes. Thanks 👍 |
loader-utilswebpack-contribstandardtest/node_modules/*because that's annoying in vscode :)